Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: add initial sniffs for modifier keyword ordering rules #2102

Merged
merged 2 commits into from
Oct 29, 2022

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 29, 2022

Core: move rules related to property/method modifier order from Extra to Core

  1. When using multiple modifiers for a property or method declaration, the order should be as follows:
    1. First the optional abstract or final keyword.
    2. Next a visibility keyword.
    3. Lastly, the optional static [red: or readonly] keyword.

For property declarations, this is already covered by the PSR2.Classes.PropertyDeclaration sniff.
For method declarations, the PSR2.Methods.MethodDeclaration sniff has now been moved from Extra to Core.

The PSR2.Methods.MethodDeclaration sniff contains one additional error code, which IMO should probably not (yet) go into the Core ruleset.

  • Underscore - which warns against prefixing a method name with an underscore, advising to use visibility instead.
    While I'm 100% in favour of this, introducing this for WP Core would be problematic as renaming (public/protected) methods would be a BC-break.

As this rule was previously already included in the Extra ruleset, the error code which is being silenced for Core, will be "unsilenced" again for Extra.

The PSR2.Classes.PropertyDeclaration sniff does not yet account for ordering the visibility vs readonly keywords.
I have opened a PR upstream - squizlabs/PHP_CodeSniffer#3637 - proposing to add a check for this to the PSR2.Classes.PropertyDeclaration sniff.

  • If that PR would be accepted, WPCS will automatically inherit the check and we'd be good.
  • If that PR would be rejected, I propose to create a dedicated sniff in PHPCSExtra to handle property modifier keywords with configuration options for the preferred order.

Refs:

Core: move last modifier keyword sniff from Extra to Core

While this was not explicitly mentioned in the Make post, it is implied (via the code samples) that there should be exactly one space after each modifier keyword for properties and methods.

This was already being checked for in Extra. I'm proposing to move this sniff to the Core ruleset.

Note: I've checked and introducing this sniff to the Core ruleset does not yield any issues for WP Core as is, so it would be a silent introduction.

Refs:

jrfnl added 2 commits October 29, 2022 13:51
…a` to `Core`

> 1. When using multiple modifiers for a property or method declaration, the order should be as follows:
>     1. First the optional `abstract` or `final` keyword.
>     2. Next a visibility keyword.
>     3. Lastly, the optional `static` [red: or `readonly`] keyword.

For property declarations, this is already covered by the `PSR2.Classes.PropertyDeclaration` sniff.
For method declarations, the `PSR2.Methods.MethodDeclaration` sniff has now been moved from `Extra` to `Core`.

The `PSR2.Methods.MethodDeclaration` sniff contains one additional error code, which IMO should probably not (yet) go into the `Core` ruleset.
* `Underscore` - which warns against prefixing a method name with an underscore, advising to use visibility instead.
    While I'm 100% in favour of this, introducing this for WP Core would be problematic as renaming (`public`/`protected`) methods would be a BC-break.

As this rule was previously already included in the `Extra` ruleset, the error code which is being silenced for `Core`, will be "unsilenced" again for `Extra`.

The `PSR2.Classes.PropertyDeclaration` sniff does not yet account for ordering the visibility vs readonly keywords.
I have opened a PR upstream - squizlabs/PHP_CodeSniffer 3637 - proposing to add a check for this to the `PSR2.Classes.PropertyDeclaration` sniff.
* If that PR would be accepted, WPCS will automatically inherit the check and we'd be good.
* If that PR would be rejected, I propose to create a dedicated sniff in PHPCSExtra to handle property modifier keywords with configuration options for the preferred order.

Refs:
* https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/ - Visibility should always be declared section
* https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#visibility-and-modifier-order
* WordPress/wpcs-docs 108
* WordPress/WordPress-Coding-Standards 1101
* WordPress/WordPress-Coding-Standards 1103
* squizlabs/PHP_CodeSniffer 3637
While this was not explicitly mentioned in the Make post, it is implied that there should be exactly one space after each modifier keyword for properties and methods.

This was already being checked for in `Extra`. I'm proposing to move this sniff to the `Core` ruleset.

Note: I've checked and introducing this sniff to the `Core` ruleset does not yield any issues for WP Core as is, so it would be a silent introduction.

Refs:
* WordPress/WordPress-Coding-Standards 1101
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GaryJones GaryJones merged commit 13cc7f6 into develop Oct 29, 2022
@GaryJones GaryJones deleted the feature/core-add-rules-modifier-order branch October 29, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants